Game of Life Rework #4995
Game of Life Rework #4995
Conversation
RAM and speed optimizations. Better repeat detection. Mutation toggle. Blur option added.
WalkthroughReplaces ColorCount with a public Cell bitfield and a contiguous Cell array in the 2D Game of Life; removes CRC-based repetition detection; adds edge/oscillator/spaceship flags; rewrites setup and evolution logic; updates the mode’s metadata string. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
5289-5303: Early break both loops when neighbors > 3 (micro-optim).break exits only the inner loop; the outer i-loop continues. Short‑circuit both loops to save work on dense patterns.
- for (int i = 1; i >= -1; i--) for (int j = 1; j >= -1; j--) if (i || j) { + bool done = false; + for (int i = 1; i >= -1 && !done; i--) for (int j = 1; j >= -1; j--) if (i || j) { int nX = x + j, nY = y + i; if (cell.edgeCell) { nX = (nX + cols) % cols; nY = (nY + rows) % rows; } unsigned nIndex = nX + nY * cols; Cell& neighbor = cells[nIndex]; if (neighbor.alive) { - if (++neighbors > 3) break; + if (++neighbors > 3) { done = true; break; } if (!neighbor.toggleStatus) { // Alive and not dying parentIdx[aliveParents++] = nIndex; } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX.cpp
🧬 Code graph analysis (1)
wled00/FX.cpp (3)
wled00/FX_fcn.cpp (2)
blur(1071-1097)blur(1071-1071)wled00/fcn_declare.h (3)
hw_random8(433-433)hw_random8(434-434)hw_random8(435-435)wled00/colors.cpp (1)
color_blend(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (1)
wled00/FX.cpp (1)
5355-5356: Metadata: surface fits behavior; confirm slider wiring.Blur is mapped from custom1 and mutation from check3 in code; metadata declares "Blur" and "Mutation" accordingly. Looks consistent. Please confirm mobile UI ties custom1/check3 to these controls.
Yes. 2D is built on top of 1D functions. |
Adjust mutation logic. Use 1D get/set. Reduce code size.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX.cpp
🧠 Learnings (1)
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
wled00/FX.cpp
🧬 Code graph analysis (1)
wled00/FX.cpp (3)
wled00/FX_fcn.cpp (2)
blur(1071-1097)blur(1071-1071)wled00/fcn_declare.h (3)
hw_random8(433-433)hw_random8(434-434)hw_random8(435-435)wled00/colors.cpp (1)
color_blend(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
| typedef struct Cell { | ||
| uint8_t alive : 1, faded : 1, toggleStatus : 1, edgeCell: 1, oscillatorCheck : 1, spaceshipCheck : 1, unused : 2; | ||
| } Cell; |
There was a problem hiding this comment.
Restore compile-time guard for Cell packing.
We still rely on the bitfield layout being 1 byte, but no longer assert it. Without the static_assert(sizeof(Cell) == 1) we risk miscompilation on other toolchains. Please reintroduce the guard right after the struct definition.
🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 5199-5201, the Cell bitfield struct relies on a
1-byte layout but the compile-time check was removed; reintroduce a
static_assert immediately after the struct definition to ensure sizeof(Cell) ==
1 (with a brief message like "Cell must be 1 byte") so builds on other
toolchains fail early if packing changes.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
while that compile time check does not hurt, it really is redundant in WLED context: ESP compiler toolchain does pack it as 1 byte. If moving to a different toolchain where this is not the case, there is a lot more issues in the code...
Thanks, swapped to the 1D version and with a few other small changes reduced code size by about 80 bytes. New version now just ~40 bytes larger. |
|
Thanks for the update @Brandon502 I'm not an expert in C++ so if you could have a look at all of code rabbits comments and put a reply, it isn't always right and some of it's suggestions are rubbish, but by you making a comment, I at least know you've revived that particular comment |
|
@netmindz I did not test the code but checked rabbit's comments. All is good IMO. |
Sorry for the delay. Seems like others have already covered the coderabbit's comments. Clamping the glider length is also not needed since it wouldn't work regardless if clamped or not for large odd shaped grids. |
| // update only when appropriate time passes (in 42 FPS slots) | ||
| if (paused || (strip.now - SEGENV.step < 1000 / map(SEGMENT.speed,0,255,1,42))) { | ||
| // Redraw if paused or between updates to remove blur | ||
| for (unsigned i = maxIndex; i--; ) { |
There was a problem hiding this comment.
@Brandon502 is this correct? maxIndex is cols*rows which is 1 beyond maximum available index.
There was a problem hiding this comment.
Figured it out. Sorry, wasn't obvious on 1st glance.
There was a problem hiding this comment.
@blazoncek Yeah, sorry some of the loops are a bit confusing. I made some of the 2D loops 1D and reversed to save flash space when they didn't need to be efficient.
4 participants
A lightweight version of my MM version to keep code size differences as small as possible (adds ~120 Bytes). Uses 1 Byte per pixel instead of 3. Testing uncapped speeds (esp32dev hub75 build) it ran about 50% faster on 32x32 and 64x32. Improved repeat detection that detects a majority of gliders and oscillators. Not 100% perfect on smaller grids that can produce long repeating patterns due to wrap, I kept mutation as a toggle option for these sizes (8x8 for example). Added blur slider to fade out dying cells.
Previous version had a max update speed of ~10 per second. I increased it to 42 like the old comment suggested.
Videos showing blur:
GoL.64x64.mp4
GoL.Blend.mp4
Patterns that are not detected:
8x8 has oscillators that repeat every 48 and 132 frames. Detection works on patterns <32 for 8x8
24x16 has a spaceship that repeats every 2448 frames. Detection works on patterns <196 for 24x16
Are 2D effects allowed to use 1D version of get/setPixelColor? I didn't include them in this PR but I did test them. They seemed to work fine and reduced code size a bit since in certain places I calculate x and y from an index. For example:
Summary by CodeRabbit